-
Notifications
You must be signed in to change notification settings - Fork 783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: typescript interface updates #973
Conversation
axe.d.ts
Outdated
|
||
type resultGroups = "inapplicable" | "passes" | "incomplete" | "violations"; | ||
type ResultGroups = "inapplicable" | "passes" | "incomplete" | "violations"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an enum
? That way we get better intellisense/etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, when this definition was first written support for enums wasn't good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right @marcysutton enums got introduced in ts 2.4 I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One quick question about ResultGroups
, but this LGTM! Thanks for the quick fix 💯
axe.d.ts
Outdated
|
||
type resultGroups = "inapplicable" | "passes" | "incomplete" | "violations"; | ||
type ResultGroups = "inapplicable" | "passes" | "incomplete" | "violations"; | ||
|
||
type RunOnlyObject = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a confusing name if it isn't being used in runOnly (only in ElementContext)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be a breaking change to rename it tho, right?
axe.d.ts
Outdated
|
||
type TagValue = "wcag2a" | "wcag2aa" | "section508" | "best-practice"; | ||
enum TagValue { | ||
wcag2a = "wcag2a", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break users' code if they were previously using TagValue
s.
For example, we're no longer able to call axewebdriverjs#withTags(['wcag2a'])
, but instead, will need to do axewebdriverjs#withTags([ axe.TagValue.wcag2a ])
.
Are we comfortable making a breaking change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The move to enum
s is a breaking change. We either need to be comfortable with a new major release, or we need to figure out a different solution.
@stephenmathieson But in my experience, that is the idea of typings. I have rolled back to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Reverted enum changes, and fixed runOnly interface
PR Checklist
Please check if your PR fulfills the following requirements:
Description of the changes
axe.RunOnly
#972Does this PR introduce a breaking change?
Other information